fix: record venue credit time on deposit archive rows - #110
Conversation
Deposit rows in broker_execution.transfer_events always landed with a NULL exchange_timestamp. Both deposit producers guarded the value with `typeof creditedAt === "string"`, but Binance reports the credit time as an integer epoch-ms (insertTime, surfaced by ccxt as `timestamp`), so the venue's own record of when it credited the funds was silently discarded and consumers had to parse it back out of payload_json. Both producers now run the value through the archive's existing timestamp normalizer, which accepts an ISO-8601 string or an epoch-ms number and emits ISO-8601 UTC — the form the forwarder configures ClickHouse to parse. Passing an epoch-ms integer through unconverted would be read as seconds and place the row tens of thousands of years ahead, so the conversion is required, not cosmetic. Covers the periodic deposit poller and the execute-action deposit handler, which write the same observe_deposit row shape into the same column; fixing only one would leave the column's contents dependent on which path observed the deposit. The field-preference order is unchanged. Rows already written are unaffected — this changes what new observations record, and no backfill is included. Existing tests fixed creditedAt as a string, which is why the defect passed a green suite; both paths now have integer coverage.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
outerlook
changed the title
1 participant
Problem
Every deposit row written to
broker_execution.transfer_eventslanded withexchange_timestamp = NULL.Both deposit producers guarded the value with
typeof creditedAt === "string". Binance reports the credit time as an integer epoch-ms (insertTime, surfaced by ccxt astimestamp), so the guard silently discarded it. The archive's own column for "when the venue credited the funds" was therefore always empty, and any consumer needing credit time — interval attribution, reconciliation, any later backfill — had to reach intopayload_jsonby hand, which is exactly the raw-JSON coupling the typed columns exist to prevent.Change
Both producers now pass the value through the archive's existing timestamp normalizer, which accepts either an ISO-8601 string or an epoch-ms number and emits ISO-8601 UTC.
ISO-8601 is the required representation, not a preference: the field is typed
string, the row builder passes it through untouched, every other producer (withdrawal submit, withdrawal observation, balance and fill pollers) already emits that form, and the archive forwarder configures ClickHouse withdate_time_input_format: best_effortspecifically to parse it. Handing theDateTime64column a raw epoch-ms integer would have it interpreted as seconds, placing the row tens of thousands of years in the future — a silent corruption worse than the NULL it replaced.Two producers are fixed because both write the same
observe_depositrow shape into the same column:Fixing only one would leave the column's contents dependent on which path happened to observe the deposit.
The field-preference order is deliberately unchanged; only the coercion changes.
Scope
Rows already written are unaffected. This changes what new observations record — there is no backfill here, so historical rows keep
exchange_timestamp = NULLand any verification query must be scoped by timestamp or they will read as failures.Tests
Both paths gain integer coverage. The existing fixtures pinned
creditedAtas a string, which is precisely why this defect passed a green suite for so long; those fixtures are kept and the integer shape is added alongside them. Both new tests were confirmed to fail against the unfixed code before being kept.Full suite: 603 passing, 0 failing.
tscandbiomeclean.Release status
This is producer-side: merged is not released, and released is not deployed. The change only takes effect in production after the cex-broker release chain runs — release PR and version tag, then the fiet-tee pin, then the fiet-maker submodule and root lockfile, then the SGX image build, then deploy.
Acceptance is a production deposit row observed after that deploy carrying a non-NULL
exchange_timestampthat differs frombroker_observed_timestamp, verified on real rows rather than in tests alone.Stack created with GitHub Stacks CLI • Give Feedback 💬